TDD assessment: PR #530 / issue #529

Verdict: changes requested

The implementation covers the main cohort/session/checkpoint design and its focused suite passes, but an interrupted run after a successful early checkpoint can omit the already-resolved discussion outcome from reviewer prompts on recovery. That violates the explicit requirement that reused reviewers receive discussion outcomes.

Strengths

• PR-scoped ordered cohort persistence, runtime compatibility checks, exact reviewer session checkpoint/resume, and --fresh-session replacement are covered in pipeline and ledger tests.
• Thread analysis is sequential and chains the orchestrator provider session; checkpoint posting validates head/base, posts reply before resolve, and keeps the run open.
• The lifecycle document, README link, and command-help assertions clearly describe rerun, fresh-session, recovery, and cr sessions scope.

Findings

High — post-checkpoint recovery drops discussion outcomes before reviewer execution

analyzeReviewThreads returns no responses once the authoritative PR summary marker suppresses the completed thread (pipeline.go:1289-1317). checkpointThreadResponses then returns immediately without loading already-persisted checkpoint actions when responses are empty (pipeline.go:1324-1327). The reviewer prompt only includes discussion_outcomes when responses are nonempty (prompts.go:43-45). Therefore, if the reply is posted and the process dies before reviewers, recovery correctly avoids duplicate analysis but reruns reviewers without the required outcome/status context.

Fix: reconstruct the outcome/status from persisted checkpoint actions (or persist a small durable outcome record) when remote markers suppress reanalysis, and pass it to resumed reviewers.

Medium — no end-to-end test proves the authoritative-marker interruption contract

Existing tests independently cover thread sequencing/checkpointing, cohort session reuse, and the reviewrun partial-checkpoint continuation, but not one real pipeline/reviewrun scenario: post a summary marker, interrupt before reviewers, recover the same run, assert no second thread LLM call/post, and assert the reviewer prompt includes the original outcome with reconciled status. This is the acceptance boundary most likely to regress.

Low — ordinary follow-up and --rerun coverage is indirect

TestDefaultSessionPersistsCohortAndResumesReviewerOnLiveRerun starts with a dry run and invokes pipeline.Live directly. Add command/reviewrun-level tests for both a normal later invocation and --rerun, asserting one fixed cohort, exact reviewer and orchestrator resume IDs, and no new selection. Add the paired --fresh-session --rerun test asserting both are reset.

Recommended tests/fixes

1. Add the high-severity same-run interruption test above; fix recovery so it preserves discussion outcomes.
2. Add a PR-marker-authoritative test with stale/missing local task data to confirm marker recovery does not duplicate reply/resolve and still gives reviewers the outcome.
3. Add reviewrun command-path tests for normal follow-up, --rerun, and --fresh-session --rerun session/cohort semantics.

Commands run

rtk git status --short --branch
rtk git diff --stat main...HEAD
rtk git diff --name-status main...HEAD
rtk go test ./internal/ledger ./internal/outbox ./internal/pipeline ./internal/reviewrun ./internal/threadanalysis ./internal/cmd/reviewcmd ./internal/cmd/sessionscmd
rtk make test
rtk go test -race ./internal/ledger ./internal/outbox ./internal/pipeline ./internal/reviewrun ./internal/threadanalysis
rtk git diff --check main...HEAD

All test commands passed; the focused command reported 532 passing tests. No repository files were changed.

Addressed the Terra assessment in 5f1acc5. Recovery now reconstructs reviewer discussion context from the authoritative PR summary marker even when local thread task/action state is missing, reports it as posted, avoids re-analysis/duplicate posting, and preserves persisted action details when available. Added crash/resume and marker-only regressions; focused suites and targeted race tests pass.

Empirical verification (feature binary, disposable draft PR #531):

• Fresh baseline run 718fde73-8b25-4b1b-bd7e-bc128476146d selected go:implementation-tests + session-probe:division, created durable reviewer sessions 019fbb13-fa20-76a0-84aa-0a15791f48d9 and 019fbb13-fa14-7791-9bdd-c7887e0389d0, and chained selection -> rollup on orchestrator 019fbb13-ba80-7f80-ae2d-f234577f58c0.
• --rerun run 8dea2464-633b-43e7-a25c-d19cd4ff95f1 logged reused reviewer cohort, omitted selection, and resumed both exact reviewer IDs. I sent SIGINT after planning and before posting.
• A plain cr review resumed that exact run ID with no LLM/selection work and completed its pending posts (posted=2, pending=0).
• Final rerun f266e01d-fd1a-4e96-8c87-e07853371c0e again omitted selection, resumed both exact reviewer IDs, and resumed the original orchestrator ID exactly.
• --fresh-session replaced the earlier cohort/orchestrator IDs before the corrected baseline.

The empirical runs caught and drove fixes for Codex reviewer workspace resume, ephemeral reviewer starts, ephemeral orchestrator starts, and dossier-session displacement of the orchestrator chain. Targeted tests cover each regression.

Limitation: the disposable profile posts as my Git identity, so it cannot manufacture a newer reply from a distinct human identity without interactive GitHub App credentials. PR-marker crash recovery, early reply ordering, and duplicate suppression are covered by pipeline/outbox/acceptance tests plus the one-time Terra assessment.